improve: blocklist of problematic resources for previous version annotation #2774
Conversation
Signed-off-by: Attila Mészáros <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
csviri
changed the title
...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
csviri
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| * | ||
| * @return blocklist of resource classes where the previous version annotation won't be used. | ||
| */ | ||
| default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() { |
There was a problem hiding this comment.
I would suggest also documenting the default implementation in the javadoc.
There was a problem hiding this comment.
Not sure that is needed / that helpful since users can just open the code and see the defaults, also we don't do it for other configs.
There was a problem hiding this comment.
What would make more sense, though, is to explain what is the consequence for a resource type to be in the block list because this isn't clear at all and people might be hesitant adding a resource to the block list without more details of what happens when they do.
There was a problem hiding this comment.
Also, this should probably be a set rather than a list as the order is meaningless whereas we only want one instance of a given resource type in this collection.
| * | ||
| * @return blocklist of resource classes where the previous version annotation won't be used. | ||
| */ | ||
| default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() { |
There was a problem hiding this comment.
What would make more sense, though, is to explain what is the consequence for a resource type to be in the block list because this isn't clear at all and people might be hesitant adding a resource to the block list without more details of what happens when they do.
| * @return blocklist of resource classes where the previous version annotation won't be used. | ||
| */ | ||
| default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() { | ||
| return List.of(Deployment.class, StatefulSet.class); |
There was a problem hiding this comment.
Another aspect is whether or not these 2 classes should always be added to the block list regardless of what the user might set this list to (i.e. what is returned by this method should be added to the default list, not replace it completely).
| * | ||
| * @return blocklist of resource classes where the previous version annotation won't be used. | ||
| */ | ||
| default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() { |
There was a problem hiding this comment.
Also, this should probably be a set rather than a list as the order is meaningless whereas we only want one instance of a given resource type in this collection.
| @@ -188,6 +190,12 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC | |||
| return this; | |||
| } | |||
|
|
|||
| public ConfigurationServiceOverrider previousAnnotationUsageBlocklist( | |||
| List<Class<? extends HasMetadata>> previousAnnotationUsageBlacklist) { | |||
| this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist; | |||
There was a problem hiding this comment.
See the previous comment on how this should probably be adding to the default set rather than completely replace it.
| .build(); | ||
| } | ||
|
|
||
| // for testing purposes replicating the matching logic but with the special matcher |
There was a problem hiding this comment.
This should use the configuration option from #2760 instead.
Signed-off-by: Attila Mészáros [email protected]